-
Notifications
You must be signed in to change notification settings - Fork 1.7k
GH-2702: RetryableTopic with asyncAcks #2706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Resolves spring-projects#2702 When using `asyncAcks` with manual ack modes, the `DefaultErrorHandler` must have `seekAfterError` set to `false`; this required user configuration. The framework now unconditionally sets the property when it configures a container using a manual ack mode. In addition, the default DLT handler was not compatible with any manual ack mode, regardless of the `asyncAcks` setting. Add `Acknowledgment` to the `LoggingDltListenerHandlerMethod`. Also tested with reporter's reproducer. **cherry-pick to 2.9.x (will require instanceof polishing for Java 8)**
| invokeDelegateOnMessage(consumerRecord, acknowledgment, consumer); | ||
| Acknowledgment ack = acknowledgment; | ||
| if (ack == null) { | ||
| // The default DLT handler now requires an Acknowledgment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I see it is @Nullable there. So, what is wrong to continue to use acknowledgment as it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of this
Lines 370 to 372 in 8efd15f
| catch (org.springframework.messaging.converter.MessageConversionException ex) { | |
| throw checkAckArg(acknowledgment, message, new MessageConversionException("Cannot handle message", ex)); | |
| } |
and this
Lines 386 to 392 in 8efd15f
| private RuntimeException checkAckArg(@Nullable Acknowledgment acknowledgment, Message<?> message, Exception ex) { | |
| if (this.hasAckParameter && acknowledgment == null) { | |
| return new ListenerExecutionFailedException("invokeHandler Failed", | |
| new IllegalStateException("No Acknowledgment available as an argument, " | |
| + "the listener container must have a MANUAL AckMode to populate the Acknowledgment.", | |
| ex)); | |
| } |
I changed the reporter's reproducer to use default AckMode and hit this problem.
I didn't think it was worth it to remove the null check in the method, but I can do so if you insist,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm - maybe I could see if the parameter is annotated @Nullable and skip that check - will look at it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I would have to re-invoke the method in that case, since the invoke already failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: I can check the nullability before the first invocation.
|
It doesn't cherry-pick clearly: too many conflicts around deprecated utils over there in |
|
Back ported as 4401f96 |
Resolves #2702
When using
asyncAckswith manual ack modes, theDefaultErrorHandlermust haveseekAfterErrorset tofalse; this required user configuration.The framework now unconditionally sets the property when it configures a container using a manual ack mode.
In addition, the default DLT handler was not compatible with any manual ack mode, regardless of the
asyncAckssetting.Add
Acknowledgmentto theLoggingDltListenerHandlerMethod.Also tested with reporter's reproducer.
cherry-pick to 2.9.x (will require instanceof polishing for Java 8)